Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: extend api with iterator for routes #41

Merged
merged 4 commits into from
Jun 6, 2024

Conversation

mbodmer
Copy link
Contributor

@mbodmer mbodmer commented Jun 6, 2024

Forward iterator access to the routes to allow collecting them as a client. This allows eg. collecting the routes to subscribe them when using the router in an MQTT usecase.

Forward iterator access to the routes to allow collecting
them as a client. This allows eg. collecting the routes
to subscribe them when using the router in an MQTT usecase.

Signed-off-by: Marc Bodmer <[email protected]>
@fundon
Copy link
Member

fundon commented Jun 6, 2024

Thank you.

Maybe implement IntoIterator will be better.
You can checkout the result of CI.

@fundon fundon self-requested a review June 6, 2024 13:38
clippy requests an implementation of into_iter()
when implementing iter()

Signed-off-by: Marc Bodmer <[email protected]>
@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

Wow, thanks for your fast action. I've also forwarded into_iter() now

@fundon
Copy link
Member

fundon commented Jun 6, 2024

See https://github.com/viz-rs/path-tree/actions/runs/9402026412/job/25895157980?pr=41#step:4:241
You might just need to implement this trait https://doc.rust-lang.org/std/iter/trait.IntoIterator.html

168 + 
169 + impl IntoIterator for &PathTree<T> {
170 +     type IntoIter = core::slice::Iter<'_, (T, alloc::vec::Vec<parser::Piece>)>;
171 +     type Item = &(T, alloc::vec::Vec<parser::Piece>);
172 +     fn into_iter(self) -> Self::IntoIter {
173 +         self.iter()
174 +     }
175 + }

@fundon
Copy link
Member

fundon commented Jun 6, 2024

I'm interested in your MQTT case. Could you add a test case?

@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

See https://github.com/viz-rs/path-tree/actions/runs/9402026412/job/25895157980?pr=41#step:4:241 You might just need to implement this trait https://doc.rust-lang.org/std/iter/trait.IntoIterator.html

168 + 
169 + impl IntoIterator for &PathTree<T> {
170 +     type IntoIter = core::slice::Iter<'_, (T, alloc::vec::Vec<parser::Piece>)>;
171 +     type Item = &(T, alloc::vec::Vec<parser::Piece>);
172 +     fn into_iter(self) -> Self::IntoIter {
173 +         self.iter()
174 +     }
175 + }

So instead of forwarding into_iter() from the vector you would prefer the implementation of the trait?

impl<T> IntoIterator for PathTree<T> {
    type Item = (T, Vec<Piece>);
    type IntoIter = IntoIter<(T, Vec<Piece>)>;

    fn into_iter(self) -> Self::IntoIter {
        self.routes.into_iter()
    }
}

@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

I'm interested in your MQTT case. Could you add a test case?

It is not a 100% fit. The "problem" was that there is no root element in MQTT, as there is no leading "/" in a topic. I have used the workaround to prefix the route with "MQTT/". Would have preferred a direct usage, but this is ok.

Also not all matchers are used. The following code create a rumqttc SubscribeFilter from the Pieces:

// Newtype for rumqttc subscription filter to implement TryFrom locally
pub struct SubscribeFilter(rumqttc::v5::mqttbytes::v5::Filter);

impl Deref for SubscribeFilter {
    type Target = rumqttc::v5::mqttbytes::v5::Filter;

    fn deref(&self) -> &Self::Target {
        &self.0
    }
}

/// Generates MQTT subscription from `TreePath` pieces
impl TryFrom<(&Vec<Piece>, &QoS)> for SubscribeFilter {
    type Error = Error;

    fn try_from(value: (&Vec<Piece>, &QoS)) -> Result<Self, Self::Error> {
        let mut bytes = Vec::new();
        let qos = *value.1;

        value.0.iter().try_for_each(|piece| match piece {
            Piece::String(s) => {
                bytes.extend_from_slice(s);
                Ok(())
            }
            Piece::Parameter(_, kind) => match kind {
                // single level wildcard, use like: "a/:name/b -> "a/+/b"
                Kind::Normal => {
                    bytes.extend_from_slice(b"+");
                    Ok(())
                }
                // multi level wildcard, use like: "a/:name*" -> "a/#"
                Kind::ZeroOrMore | Kind::ZeroOrMoreSegment => {
                    bytes.extend_from_slice(b"#");
                    Ok(())
                }
                // fail in other cases
                _ => Err(Error::ParseError(
                    "Unsupported route expression".to_string(),
                )),
            },
        })?;

        // construct topic from bytes and remove prefix if appliable
        let mut topic = from_utf8(&bytes).map_err(|e| Error::ParseError(e.to_string()))?;
        topic = topic.strip_prefix("MQTT/").unwrap_or(topic);

        Ok(SubscribeFilter(rumqttc::v5::mqttbytes::v5::Filter::new(
            topic, qos,
        )))
    }
}

Sure i can implement a testcase, do you have any pointers on what you have in mind?

@fundon
Copy link
Member

fundon commented Jun 6, 2024

So instead of forwarding into_iter() from the vector you would prefer the implementation of the trait?

Yes, like clippy said: https://rust-lang.github.io/rust-clippy/master/index.html#/iter_without_into_iter

@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

could you please restart the build one more time?

@fundon
Copy link
Member

fundon commented Jun 6, 2024

It is not a 100% fit. The "problem" was that there is no root element in MQTT, as there is no leading "/" in a topic. I have used the workaround to prefix the route with "MQTT/". Would have preferred a direct usage, but this is ok.

Thank you for sharing.
Yes, a leading is required. It's fine.

@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

Thank you for this library, it still was very useful

@fundon
Copy link
Member

fundon commented Jun 6, 2024

The CI's output:

help: consider implementing `IntoIterator` for `&PathTree<T>`
    |
168 + 
169 + impl IntoIterator for &PathTree<T> {
170 +     type IntoIter = core::slice::Iter<'_, (T, alloc::vec::Vec<parser::Piece>)>;
171 +     type Item = &(T, alloc::vec::Vec<parser::Piece>);
172 +     fn into_iter(self) -> Self::IntoIter {
173 +         self.iter()
174 +     }
175 + }

@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

See https://github.com/viz-rs/path-tree/actions/runs/9402026412/job/25895157980?pr=41#step:4:241 You might just need to implement this trait https://doc.rust-lang.org/std/iter/trait.IntoIterator.html

168 + 
169 + impl IntoIterator for &PathTree<T> {
170 +     type IntoIter = core::slice::Iter<'_, (T, alloc::vec::Vec<parser::Piece>)>;
171 +     type Item = &(T, alloc::vec::Vec<parser::Piece>);
172 +     fn into_iter(self) -> Self::IntoIter {
173 +         self.iter()
174 +     }
175 + }

Hmm, i have implemented it, but clippy seems not to accept it: https://github.com/viz-rs/path-tree/pull/41/files#diff-b1a35a68f14e696205874893c07fd24fdb88882b47c23cc0e0c80a30c7d53759R267
The example provided by clippy from ci does not work, and my local clippy does not complain.
Do you have any idea?

Edit: I have now implemented IntoIterator for &PathTree, hope this works on CI

@fundon
Copy link
Member

fundon commented Jun 6, 2024

Thank you for your work.

@fundon fundon merged commit ff63bd3 into viz-rs:main Jun 6, 2024
6 checks passed
@fundon
Copy link
Member

fundon commented Jun 6, 2024

Laned in v0.8.0.

@mbodmer mbodmer mentioned this pull request Jun 6, 2024
@mbodmer
Copy link
Contributor Author

mbodmer commented Jun 6, 2024

Thanks for releasing, cool!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants